-
Notifications
You must be signed in to change notification settings - Fork 438
Add remote_addr field to lightning_net_tokio::setup_outbound
#4372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add remote_addr field to lightning_net_tokio::setup_outbound
#4372
Conversation
tankyleo
commented
Feb 2, 2026
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
Fixes the issues @jharveyb reported in lightningdevkit/ldk-node#778 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4372 +/- ##
==========================================
+ Coverage 85.98% 86.05% +0.07%
==========================================
Files 156 156
Lines 102641 103207 +566
Branches 102641 103207 +566
==========================================
+ Hits 88258 88817 +559
- Misses 11873 11878 +5
- Partials 2510 2512 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tnull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally makes sense. One question, otherwise CI seems unhappy right now.
| /// [`PeerManager::list_peers`]: lightning::ln::peer_handler::PeerManager::list_peers | ||
| pub fn setup_outbound<PM: Deref + 'static + Send + Sync + Clone>( | ||
| peer_manager: PM, their_node_id: PublicKey, stream: StdTcpStream, | ||
| remote_addr: Option<SocketAddress>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, rather than adding this on the generic setup_outbound, would it make sense to refactor this to be an setup_outbound_internal and offer two setup_outbound and setup_outbound_tor variants that do the right (tm) thing for the user? Or, put differently, for non-Tor users this remote_addr argument might be confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know how this sounds below tnull, I avoid adding a setup_outbound_tor, and call setup_outbound_internal directly from connect_outbound_tor. I'd prefer to avoid adding a new function unless we really have to.
TheBlueMatt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @tnull otherwise lgtm
427b76a to
28cf5f5
Compare
tnull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, but it would be great if we could add some kind of test coverage (maybe even based on the _internal method to check that the override works as expected?
tnull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to squash!
When `setup_outbound` was used to setup a connection proxied over Tor, it previously set the remote address of the peer to the address of the Tor proxy. This address of the Tor proxy was assigned to the `PeerDetails::socket_address` for that peer in `PeerManager::list_peers`, and if it was not a private IPv4 or IPv6 address, it was also reported to the peer in our init message. This commit refactors `tor_connect_outbound` to pass its own peer address parameter directly to the connection setup code. This peer address will now appear in `PeerManager::list_peers` for outbound Tor connections made using `tor_connect_outbound`, and will be reported to the peer in our init message if it is not a private IPv4 or IPv6 address.
40f17c5 to
6f315f2
Compare
Squashed with no changes |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks. just gonna land cause its simple and you addressed @tnull's feedback.
tnull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-merge ACK